Experiment: Fluent theme#4761
Experiment: Fluent theme#4761mikewheaton merged 10 commits intomicrosoft:masterfrom mikewheaton:miwhea/fluent-experiment
Conversation
mikewheaton
requested review from
Jahnp,
dzearing,
jordandrako,
oengusmacinog-zz and
phkuo
| import { FluentColors } from "@uifabric/experiments/lib/components/fluent/theme/FluentColors"; | ||
|
|
||
| const FluentTheme: ITheme = createTheme({ | ||
| palette: { |
There was a problem hiding this comment.
This will be expanded over time, as all of the current colors (not just grays) have equivalents in the Fluent palette.
| pink10: '#edbed3', | ||
|
|
||
| // Communication | ||
| communication90: '#004578', |
There was a problem hiding this comment.
Just thinking out loud--I wonder if these palettes should be broken out as separate palette objects, e.g. CommunicationPalette, PowerPointPalette, etc.
There was a problem hiding this comment.
Great idea. I've named then like CommunicationColors to distinguish between colors (the box of crayons) and a palette (a set of named slots that can be colored in).
| @@ -0,0 +1,288 @@ | |||
| export const FluentColors: any = { | |||
There was a problem hiding this comment.
Are we supposed to have these product-specific things in Fabric?
There was a problem hiding this comment.
I'm not a fan of it myself, as it feels like it goes beyond the scope of Fabric as a shared design language. We're likely to add many more apps and color palettes over time, so there are concerns for bundle size (and all of the classes in Fabric Core) as well as maintaining this. I'll bring it up again when discussing Fluent with the designers.
There was a problem hiding this comment.
I've removed the app-specific colors, except for Communication as it covers multiple apps and we need default theme colors.
| <a href="https://fluent.microsoft.com/">Fluent Design System</a>. | ||
| Before these colors become the defaults, this theme is provided as | ||
| a way to preview how the color changes will affect your app. Note | ||
| that theming requires components to be written using{" "} |
There was a problem hiding this comment.
This isn't true, only contextual theming requires the use of mergeStyles.
There was a problem hiding this comment.
Thanks for catching this. So <Customizer/> is used for contextual theming, and loadTheme() for theming the entire app?
| mergeStyles | ||
| </a>, the Customizer component can be used to apply the theme. See | ||
| the examples below. For all other components (including your app's | ||
| custom components), use the loadTheme() function. |
There was a problem hiding this comment.
Super nit: but might be useful to readers if we link loadTheme() to the actual implementation in source, https://github.com/OfficeDev/office-ui-fabric-react/blob/2ea373d0f35d2ebba56084455987f829e6e72188/packages/styling/src/styles/theme.ts#L80
There was a problem hiding this comment.
Great idea! Added
| @@ -0,0 +1,245 @@ | |||
| /* tslint:disable:no-any */ | |||
| export const GrayColors: any = { | |||
There was a problem hiding this comment.
Does out linter allow type 'any'? In any case, maybe [key: string]: string would be better.. marginally
There was a problem hiding this comment.
any is allowed here. :)
5 participants
Pull request checklist
$ npm run changeDescription of changes
This PR adds a Fluent theme to the experiments package, as well as an example page showing how to use
Customizerto apply this theme to a component. All of the Fluent colors are included in an object for reference by any theme.Focus areas to test
npm startfrom within/packages/experiments/Screenshot